-
Notifications
You must be signed in to change notification settings - Fork 1
[24.11.28 / TASK-36] Feature - 로그인 페이지 제작 #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
놀랍게도 현재 Next 15는 RC가 아니라는거~
+ 로그인 테스트 추가 (엣지케이스만)
괄호가 붙으면 URL로써 인식되지 않는데, 이 때문에 첫 페이지가 두개인 오류 발생
버튼과 입력 추가
+ ToastContainer 추가
FE의 테스트 코드는 처음 보는데 사용자 행동을 테스팅하는군요! 잘봤습니다 좋았던 점
개선할 점
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR 잘 확인했습니다!
코멘트 남긴 모든 부분에 대해서 수정을 요청드린 건 아니니 확인하시고 기준님 판단대로 하시면 될 것 같습니다. 그러고 re-request 날려주세요~~
좋았던 점
- 하온님 말씀대로 테스트를 그룹화해주셔서 이해하기 편한 것 같습니다!
- 자주 사용되는/될 css 속성을 tailwind에서 커스텀하신 것도 앞으로 사용성 측면에서 좋다고 생각합니다👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋았던 사항
- ㅋㅋㅋㅋ벌써 많은 전쟁을 치룬 뒤에 보게 된 것 같아, 훨씬 뭐랄까 피땀눈물이 느껴진달까..? 고생많으셨어요
- FE 에서 테스트 쉽지않은데 정말로, 깔끔한 성공 / 실패 테스트케이스 인 것 같아요. 특히 랜더링 체크에서요 :)
- 이제 셀레니움등을 활용한 테스트도 (지금 전혀 필요없음) 살짝씩 체크해보시면 어떨까? 라는 생각도 듭니다 :)
개선 사항
- 개별 코멘트 한 번 확인해 주세요!
- 테스트 코드에서 "서버가 문제가 있을때" 에 대한 케이스가 없는 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/app/(login)/Content.tsx (2)
10-13
: 인터페이스 네이밍 컨벤션 개선이 필요합니다인터페이스 이름은 파스칼 케이스를 사용하는 것이 일반적입니다.
다음과 같이 수정을 제안드립니다:
-interface formVo { +interface FormData { access_token: string; refresh_token: string; }
42-67
: 코드 구조 개선이 필요합니다현재 구현에서 다음과 같은 개선 사항을 제안드립니다:
- Tailwind 클래스명이 너무 길어 가독성이 떨어집니다
- 문자열이 하드코딩되어 있어 유지보수가 어려울 수 있습니다
다음과 같은 방식으로 개선할 수 있습니다:
const FORM_STYLES = { container: "w-full h-full flex items-center justify-center", form: "w-fit h-fit flex flex-col gap-[30px] items-center p-[30px] bg-bg-2 rounded-[4px] shadow-[0_4px_16px_0_rgba(0,0,0,.04)]", title: "font-medium text-[32px] text-text-1" }; const STRINGS = { title: "Velog Dashboard", accessToken: "Access Token을 입력하세요", refreshToken: "Refresh Token을 입력하세요", login: "로그인" };이렇게 분리된 상수를 컴포넌트에서 사용하면 코드의 가독성과 유지보수성이 향상됩니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.env.sample
(1 hunks)src/app/(login)/Content.tsx
(1 hunks)src/components/Input.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.sample
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Input.tsx
🔇 Additional comments (2)
src/app/(login)/Content.tsx (2)
38-40
: 폼 제출 로직이 깔끔합니다
폼 제출 로직이 간단하고 명확하게 구현되어 있습니다.
24-36
: 🛠️ Refactor suggestion
토큰 전송 방식 개선이 필요합니다
현재 구현에서 몇 가지 개선이 필요해 보입니다:
- 쿠키를 직접 헤더에 설정하는 것보다 서버에서 Set-Cookie 헤더를 통해 설정하는 것이 더 안전합니다.
- 에러 메시지가 사용자 친화적이지 않습니다.
다음과 같이 수정을 제안드립니다:
const { mutate } = useMutation({
mutationFn: async (data: formVo) =>
await instance('/login', {
method: 'POST',
- headers: {
- cookie: `access_token=${data.access_token};refresh_token=${data.refresh_token}`,
- },
+ data: {
+ access_token: data.access_token,
+ refresh_token: data.refresh_token,
+ },
}),
onSuccess: () => replace('/main'),
onError: (res: Response) => {
- toast.error(`${res.statusText} (${res.status})`);
+ toast.error('로그인에 실패했습니다. 토큰을 확인해 주세요.');
},
});
Likely invalid or redundant comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
eslint.config.mjs
(1 hunks)tsconfig.json
(1 hunks)
🔇 Additional comments (2)
eslint.config.mjs (2)
27-29
:
TypeScript 규칙 설정 검토 필요
TypeScript 규칙 중 일부는 강화되었지만, naming-convention
규칙이 비활성화된 것이 우려됩니다. 네이밍 컨벤션은 코드 일관성과 가독성에 중요한 역할을 합니다.
다음과 같이 수정하는 것을 제안드립니다:
'@typescript-eslint/promise-function-async': 'error',
'@typescript-eslint/consistent-type-assertions': 'error',
- '@typescript-eslint/naming-convention': 'off',
+ '@typescript-eslint/naming-convention': 'error',
31-35
: TypeScript 프로젝트 설정 검증 필요
parserOptions
의 설정이 올바르게 되어있는지 확인이 필요합니다.
다음 스크립트를 실행하여 TypeScript 설정을 검증해보세요:
✅ Verification successful
Let me gather more information about the ESLint configuration and TypeScript setup.
TypeScript와 ESLint 설정이 올바르게 구성되어 있습니다
TypeScript와 ESLint 설정이 다음과 같이 적절하게 구성되어 있음을 확인했습니다:
tsconfig.json
이 존재하며 올바른 컴파일러 옵션이 설정되어 있습니다- ESLint 설정에서 TypeScript 관련 플러그인과 규칙이 제대로 구성되어 있습니다
parserOptions
의project: true
와tsconfigRootDir
설정이 TypeScript 프로젝트 구조에 맞게 올바르게 설정되어 있습니다
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: TypeScript 설정 파일 존재 여부와 설정 확인
# tsconfig.json 파일 존재 여부 확인
echo "TypeScript 설정 파일 확인:"
if [ -f "tsconfig.json" ]; then
cat tsconfig.json | jq '.compilerOptions'
else
echo "tsconfig.json 파일을 찾을 수 없습니다."
fi
# ESLint 설정에서 참조하는 파일들의 존재 여부 확인
echo -e "\nESLint 플러그인 의존성 확인:"
cat package.json | jq '.dependencies + .devDependencies | with_entries(select(.key | startswith("@typescript-eslint")))'
Length of output: 730
Script:
#!/bin/bash
# Check for ESLint related dependencies and configurations
echo "ESLint 관련 의존성 확인:"
cat package.json | jq '.dependencies + .devDependencies | with_entries(select(.key | contains("eslint") or .key | contains("typescript")))'
echo -e "\nESLint 설정 파일 내용:"
cat eslint.config.mjs
echo -e "\n프로젝트 루트 디렉토리의 파일 구조:"
ls -la
Length of output: 3129
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/app/(login)/Content.tsx (3)
38-40
: 사용자 피드백 개선이 필요합니다로딩 상태를 표시하여 사용자 경험을 개선할 수 있습니다.
+ const { mutate, isLoading } = useMutation({ // ... mutation 설정 const onSubmit = (data: formVo) => { + if (isLoading) return; mutate(data); };
51-62
: 입력 필드 피드백 개선이 필요합니다사용자에게 더 명확한 피드백을 제공하기 위해 다음과 같은 개선을 제안드립니다:
<Input size="large" type="password" placeholder="Access Token을 입력하세요" - {...register('access_token', { required: true })} + {...register('access_token', { + required: '액세스 토큰을 입력해주세요' + })} + error={errors.access_token?.message} />
63-65
: 버튼 상태 표시 개선이 필요합니다로딩 상태를 버튼에 반영하여 사용자 경험을 개선할 수 있습니다:
- <Button size="large" form="large" type="submit" disabled={!isValid}> + <Button + size="large" + form="large" + type="submit" + disabled={!isValid || isLoading} + > - 로그인 + {isLoading ? '로그인 중...' : '로그인'} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/app/(login)/Content.tsx
(1 hunks)src/app/layout.tsx
(2 hunks)src/components/Button.tsx
(1 hunks)src/components/Input.tsx
(1 hunks)tailwind.config.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/layout.tsx
- src/components/Button.tsx
- src/components/Input.tsx
- tailwind.config.ts
import { Button, Input } from '@/components'; | ||
import { useForm } from 'react-hook-form'; | ||
import { useMutation } from '@tanstack/react-query'; | ||
import { instance } from '../../api'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
인터페이스 네이밍 컨벤션 개선이 필요합니다
TypeScript 네이밍 컨벤션에 따라 다음과 같이 수정을 제안드립니다:
-interface formVo {
+interface LoginFormData {
access_token: string;
refresh_token: string;
}
또한 상대 경로 대신 절대 경로를 사용하는 것이 좋습니다:
-import { instance } from '../../api';
+import { instance } from '@/api';
Also applies to: 10-13
const { mutate } = useMutation({ | ||
mutationFn: async (data: formVo) => | ||
await instance('/login', { | ||
method: 'POST', | ||
headers: { | ||
cookie: `access_token=${data.access_token};refresh_token=${data.refresh_token}`, | ||
}, | ||
}), | ||
onSuccess: () => replace('/main'), | ||
onError: (res: Response) => { | ||
toast.error(`${res.statusText} (${res.status})`); | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
보안 및 에러 처리 개선이 필요합니다
- 쿠키를 직접 헤더에 포함시키는 것은 보안상 위험할 수 있습니다.
- 사용자에게 기술적인 에러 메시지를 직접 노출하는 것은 좋지 않습니다.
다음과 같이 수정을 제안드립니다:
const { mutate } = useMutation({
mutationFn: async (data: formVo) =>
await instance('/login', {
method: 'POST',
- headers: {
- cookie: `access_token=${data.access_token};refresh_token=${data.refresh_token}`,
- },
+ data: {
+ access_token: data.access_token,
+ refresh_token: data.refresh_token,
+ },
}),
onSuccess: () => replace('/main'),
onError: (res: Response) => {
- toast.error(`${res.statusText} (${res.status})`);
+ toast.error('로그인에 실패했습니다. 다시 시도해 주세요.');
},
});
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const { mutate } = useMutation({ | |
mutationFn: async (data: formVo) => | |
await instance('/login', { | |
method: 'POST', | |
headers: { | |
cookie: `access_token=${data.access_token};refresh_token=${data.refresh_token}`, | |
}, | |
}), | |
onSuccess: () => replace('/main'), | |
onError: (res: Response) => { | |
toast.error(`${res.statusText} (${res.status})`); | |
}, | |
}); | |
const { mutate } = useMutation({ | |
mutationFn: async (data: formVo) => | |
await instance('/login', { | |
method: 'POST', | |
data: { | |
access_token: data.access_token, | |
refresh_token: data.refresh_token, | |
}, | |
}), | |
onSuccess: () => replace('/main'), | |
onError: (res: Response) => { | |
toast.error('로그인에 실패했습니다. 다시 시도해 주세요.'); | |
}, | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
힘든 싸움을 하셨군요..!! 수고하셨습니다👏🏻
"서버에 문제가 있을때" 케이스에 대해서는 리팩토링 브랜치에서 진행하도록 하겠습니다. |
import { Button, Input } from '@/components'; | ||
import { useForm } from 'react-hook-form'; | ||
import { useMutation } from '@tanstack/react-query'; | ||
import { instance } from '../../api'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 이건 진짜 바꾸면 좋긴한데 ㅋㅋㅋㅋㅋㅋㅋ next config 에서 세팅해놓고 절대 경로로 하는게 진짜! 크,, 다음 브랜치 기약 오케이 합니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생 너무많으셨어요! 리펙토링 브랜치를 기약합시다! 아자아자!
설명
간단한 디자인을 기반으로 로그인 페이지를 제작하였습니다.
진행한 작업
그 외..
Jest를 처음 사용하다 보니 조금 어색해서 오래 걸렸던 것 같습니다.
특히 jest-DOM과 mocking 작업에서 이해가 안 가는 부분이 조금 많았습니다;
Summary by CodeRabbit
릴리스 노트
새로운 기능
QueryProvider
를 통한 데이터 페칭 관리 기능 추가.sizeStyle
상수를 통한 버튼 및 입력 필드 크기 관리 추가.NEXT_PUBLIC_BASE_URL
추가.버그 수정
문서화
.env.sample
파일에 환경 변수 추가.리팩토링
스타일
테스트
기타
package.json
파일 추가.